fix(maven-plugin): resolve Maven -T parallel-build deadlock - #264
Merged
Conversation
…key fix) Root cause: concurrent first-init of independently-locked process-global registry singletons (defaultLoaderRegistry / getInstance via ConstraintEnforcer / ServiceRegistryFactory + JVM class-init) under `mvn -T` deadlocks on lock-ordering. Secondary: activeLoaders keys by name-without-sources, so two reactor modules sharing a <loader> name share one load (silent wrong output). Design (Fable-reviewed, SOUND-WITH-CHANGES): - Part A: deterministic single-thread warm-up of the three globals (RegistryBootstrap.warmUpDefaults), called from MetaDataLoader.init() (library-wide) + the mojo execute()s (Maven pre-init eager getTypeRegistry); mark generate/verify/docs threadSafe (labeling, ships atomically). - Part B: per-instance id in buildLoaderKey so dedup only coalesces the same instance. - Rejected per-loader createWithCoreProviders isolation (SPI pollution + N× cost; sealed registry is already read-only). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HLoJkFSyoticveo5ehMUAr
…invoker IT) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HLoJkFSyoticveo5ehMUAr
Two reactor modules with the same <loader> name shared one init() future (buildLoaderKey was class:subType:name, no instance discriminator) — module B's init() rode module A's future and returned A, leaving B's tree empty. Append a process-unique instanceId so dedup only coalesces the same instance. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HLoJkFSyoticveo5ehMUAr
Force-initialize the three process-global registry singletons on one thread under one lock, so a concurrent first-init cannot deadlock on their independent locks. Idempotent; failure propagates. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HLoJkFSyoticveo5ehMUAr
Call RegistryBootstrap.warmUpDefaults() at the top of MetaDataLoader.initWithConcurrencyProtection (covers every loader embedder) and the generate/docs/editor + verify mojo execute()s (covers Maven's pre-init eager getTypeRegistry() first-touch). Existing metadata (1272) + maven-plugin (24) suites stay green; behavior byte-identical. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HLoJkFSyoticveo5ehMUAr
Honest declaration now that the shared-state deadlock is fixed (warm-up + per-instance loader key). Maven 3.x does not serialize non-threadSafe mojos under -T — it only warns — so this is labeling that ships atomically with the fix, and suppresses the (now-accurate) warning. editor (direct-invocation) and agent-docs (stub) are correctly left unmarked. Descriptor test asserts it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HLoJkFSyoticveo5ehMUAr
Manual 3-module -T4 reactor (mod-a/mod-b share loader name, mod-c distinct):
pre-fix 8/8 runs fail on the shared-name module ("MetaDataLoader [shared] is
not usable. Phase: UNINITIALIZED" — the Part B collision), post-fix 5/5 pass
with per-module isolation. Declined a committed maven-invoker IT: Part B is
already deterministic in LoaderKeyIsolationTest and Part A's deadlock is
probabilistic, so the invoker infra adds mostly-redundant coverage.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HLoJkFSyoticveo5ehMUAr
…old-init limit Code review (report-only) found no correctness issues. Two minor fixes: - public-repo hygiene: drop the embedded name/email/session-URL from the plan doc's example commit blocks (commit-message trailers are unaffected). - honesty: javadoc on warmUpIsThreadSafeUnderConcurrentCallers noting it can't reproduce the cold first-init deadlock (singletons warm once per JVM); the faithful before/after is the manual -T4 reactor recorded in the design doc. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HLoJkFSyoticveo5ehMUAr
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Fix GitHub #233: Maven -T parallel-build deadlock in metaobjects-maven-plugin. Two parts. Part A (deadlock): new RegistryBootstrap.warmUpDefaults() deterministically initializes the three process-global registry singletons (MetaDataRegistry.getInstance, RegistryManifest.defaultLoaderRegistry, ConstraintEnforcer.getInstance) on a single thread under one lock before any parallel first-init can race their independent locks; called from MetaDataLoader.initWithConcurrencyProtection (all loader embedders) and the mojo execute()s (Maven's pre-init eager getTypeRegistry touch). Part B (correctness): MetaDataLoader gains a process-unique instanceId appended to buildLoaderKey() (now package-private for the test) so the static activeLoaders dedup only coalesces the same instance — two reactor modules sharing a name no longer share one load. generate/verify/docs mojos marked @mojo(threadSafe=true) as honest labeling that ships atomically (Maven 3.x does not serialize non-threadSafe mojos under -T, only warns). editor (direct-invocation) and agent-docs (stub) intentionally left unmarked. New tests: LoaderKeyIsolationTest, RegistryBootstrapTest, MojoThreadSafeDescriptorTest (reads generated plugin.xml since @mojo is CLASS-retention). Maven-only change (Java maven-plugin + metadata registry); behavior byte-identical for existing single-module builds. Verified: metadata 1272 + maven-plugin 24 + codegen-spring 187 + codegen-kotlin 310 green; manual -T4 reactor before/after — unfixed 8/8 fail (MetaDataLoader [shared] is not usable, Phase UNINITIALIZED), fixed 5/5 pass with per-module isolation. PR should Close #233.
What Changed
RegistryBootstrap.warmUpDefaults(), which deterministically initializes the three process-global registry singletons (MetaDataRegistry,RegistryManifest,ConstraintEnforcer) on one thread under a single lock before any parallel first-init can race their independent locks; invoked fromMetaDataLoader.initWithConcurrencyProtectionand each mojo'sexecute().MetaDataLoadera process-uniqueinstanceIdappended tobuildLoaderKey(), so the staticactiveLoadersdedup only coalesces the same instance — two reactor modules sharing a<loader>name no longer collapse into one shared load.generate,verify, anddocsmojos@Mojo(threadSafe=true)as honest parallel-build labeling;editorandagent-docsare intentionally left unmarked.Closes #233.
Risk Assessment
✅ Low: Well-bounded Maven-only change: the warm-up is strictly additive (idempotent single-thread serialization of an already-shared sealed registry), the per-instance key fix is internal and same-instance semantics are preserved, threadSafe is labeling-only in Maven 3.x, and all reactor entry points + the loader chokepoint warm before the singleton first-touch; behavior is byte-identical for existing single-module builds and the durable-fix invariants hold for the stated scope.
Testing
Targeted unit tests (the three new ones, all green) plus a faithful end-to-end -T4 reactor reproducer that I authored from the design doc: pre-fix base-commit plugin fails 5/5 with the exact shared-loader UNINITIALIZED collision (loser flipping between mod-a/mod-b, mod-c with a distinct name always succeeding), while the fixed tip plugin passes 6/6 with strict per-module isolation and completes without hanging — demonstrating both Part A (no deadlock under parallelism) and Part B (per-instance loader-key isolation). threadSafe mojo labeling and the warm-up call sites were additionally confirmed in the generated/installed artifacts. One setup hiccup (base build missing repo-root spec/metamodel) was fixed by re-extracting the full source tree; fixed artifacts were reinstalled and re-verified. Worktree left clean.
Evidence: Pre-fix -T4 reactor — 5/5 FAIL (exact UNINITIALIZED collision)
Pre-fix (base 6ea8deb7) under mvn -T4: 5/5 BUILD FAILURE. Loser of the shared-<loader> race errors: Execution default of goal com.metaobjects:metaobjects-maven-plugin:7.20.11:docs failed: MetaDataLoader [shared] is not usable. Phase: UNINITIALIZED (Not yet initialized), Elapsed: ~80ms, Version: 0 Runs 1-3: mod-b loses → mod-a→Alpha.md, mod-b→<none>. Runs 4-5: mod-a loses → mod-b→Bravo.md, mod-a→<none>. mod-c (distinct name) → Charlie.md every run.Evidence: Post-fix -T4 reactor — 6/6 PASS (per-module isolation, no hang)
Post-fix (tip a3ad3610) mvn -T4 clean package: mod-a .............................................. SUCCESS [ 0.506 s] mod-b .............................................. SUCCESS [ 0.503 s] mod-c .............................................. SUCCESS [ 0.503 s] BUILD SUCCESS Total time: 0.602 s (Wall Clock) real 1.034s / user 5.059s (real parallelism, no deadlock) Per-module docs: mod-a→Alpha.md, mod-b→Bravo.md, mod-c→Charlie.md; 0 cross-module entity references (content-checked).Evidence: Post-fix -T4 reactor — 5 isolation runs
Evidence: Restore-confirm -T4 pass after reinstalling fixed artifacts
Evidence: Consolidated before/after validation summary
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
✅ **Review** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
mvn -pl metadata test -Dtest='LoaderKeyIsolationTest,RegistryBootstrapTest' — 5/5 methods green (Part B key uniqueness + Part A warm-up idempotency/thread-safety)mvn -pl maven-plugin test -Dtest='MojoThreadSafeDescriptorTest' — 1/1 green (reads generated plugin.xml)plugin.xml threadSafe inspection: generate/verify/docs=true; editor/agent-docs=false (intent requires editor/agent-docs unmarked)mvn -pl maven-plugin -am install -DskipTests (fixed) + javap/strings on installed jars: RegistryBootstrap present, package-private buildLoaderKey, warmUpDefaults compiled into AbstractMetaDataMojo+MetaDataVerifyMojoAuthored 3-module reactor (mod-a+mod-b share <loader name=shared>, mod-c distinct) under mvn -T4: pre-fix (base 6ea8deb7) 5/5 FAIL with 'MetaDataLoader [shared] is not usable. Phase: UNINITIALIZED'; post-fix (tip a3ad3610) 6/6 PASS, mod-a→Alpha/mod-b→Bravo/mod-c→Charlie, 0 cross-module references, ~0.6s wall-clock (no deadlock)git archive 6ea8deb7 to build pre-fix artifacts (re-extracted with spec/metamodel after first attempt missed the resource); reinstalled fixed artifacts and confirmed -T4 passes once moregit status --short on worktree: clean (target/ gitignored); /tmp scratch dirs removed; evidence files preserved🔧 **Document** - 1 issue found → auto-fixed ✅
docs/features/cli.md:32- The change makes generate/verify/docs threadSafe=true and resolves the -T reactor deadlock — a real user-facing capability unlock (pre-fixmvn -Tdeadlocked 8/8; post-fix 5/5 pass). No documentation mentions parallel-build safety anywhere; the CLI matrix (cli.md:32-33) and server/java/README.md:70 list the goals but say nothing about-T. A one-line note in the Java CLI-matrix row (and/or the Java README) that the JVM goals support parallel reactor builds would make this discoverable. Left unfixed: this is additive (no existing fact is stale) and the repo convention records capability changes in CHANGELOG at release time, so it is a doc-policy judgment call rather than a staleness repair.🔧 Fix: docs(#233): note parallel-build safety for JVM goals
✅ Re-checked - no issues remain.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.